-
-
Notifications
You must be signed in to change notification settings - Fork 21k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implemented better way to map inputs in godot #16797
Conversation
I don't understand why people want dealing with axis instead of actions. |
This is really useful. Would simplify a lot of my personal input handling code! I would agree with groud in that naming it a little more generically just |
@groud I like |
It's a little bit more than a renaming, it's just avoiding replacing the (axis+direction) way of mapping by an axis (without direction) mapping. While it work pretty well by now. |
I also like Grouds idea for the name. Love this change @AndreaCatania Groud, do note that its not just the mapping, you have to define whether a keypress results in a positive or negative value so I like what Andrea has done here. |
Precisely, this is what I don't want, but the contrary. The previous system allowed to bind both an axis and a key to the same action with no more complex stuff. |
With this PR you can bind key and axis to the same action, you can know the action axis and also if it's pressed or released. Can you please give me an example in order to understand exactly what you mean? |
@groud the downside of that approach is that you'd need to do:
While @AndreaCatania solution would allow you to do:
That said, I don't mind the extra line of code to combine them and it would fit well in the current input mapping making the change far less impactful. For a lot of already made games you can leave everything as is but just changing get_action_pressed for get_action_value where you need analogue precision. With for instance the car simulation I'm working on throttle and brake could be one joystick axis (gamepad play) or two trigger axis (steering wheel and peddles) so the breakup into a separate throttle and breaking action makes far more sense. So yeah, I think Grouds suggestion would work out well. |
It looks nice and seems to be just what I want for my project. By the way, does it also support using mouse motion or wheel as an axis, or do we need another issue/PR to implement that? And does an axis input repeats its value when it's in the 'neutral' position, or does it work the same way as a trigger type input (emits events only when it's 'pressed')? |
Sure, it leads to more lines of code, but it's more consistent in the binding as a key cannot have a negative value. Else it would mean that you would have actions that can go into negative values and other that can't. Even if it's a little bit counter-intuitive at the beginning, it means any action can be binded to any type of input, which is, IMHO, easier to manage in the end :) |
@groud thing is, the current solution does not have a direction for key (pressed is always 1) but you have a direction for axis which is why every axis appears twice. Its not just taking the absolute of the axis, it is "for a positive axis, return the value as if when positive else 0" and "for a negative axis, return the values negated if negative else 0". Andrea just reversed that, returning the axis value as is, but allowing keys to return either 1 or -1 or assumably any value which can be handy. But don't get me wrong, the more I think of it, the more I believe your suggestion is the better one. Right now we have a separate implementation for action pressed and action value. Your suggestion might require an extra line of code, it offers more flexibility and its simpler to understand. It also prevents confusion between people who want to use actions for normal key pressed actions and for axis input. That all said, I had a quick look through the code and this change could take a bit of work. I would suggest adding a value to the Action structure and then enhancing this loop: To not just set action.pressed to true but also action.value to the value we want. It'll mean enhancing the event class to return the value for that event, which for a key event would be 0 or 1, for a joystick event would be between 0.0 and 1.0 and for mouse event i guess based on the relative mouse movement? It also means adding a 2nd parameter for the value to: And making sure: All in all it does feel like an elegant solution if done right. |
I don't understand what is the benefit to have a value between 0 and 1, since with the approach that you said you have to create always an action for each axis direction and also you have to bind keys to these. Also you have to check for each axis direction if the action occurs, and this is useless for most of the times. I don't know, but I don't see a lot of situation where split the two axis in four other make sense, or makes life easier, even because in order to support it I've to add more checks to it, and I don't like it. I think that at the end the most of the times you will end with a GDScript like this:
plus all code to split these in the engine. It add useless complexity, so for me it doesn't worth and doesn't make the life easy |
@AndreaCatania When you look at character movement in a top down map, you are absolutely right. But in many other use cases you'll fine your self doing this:
Instead I can do:
For my brake example it gets even worse. I would have to do something like:
It may seem weird to handle both accelerate and brake actions at the same time if you have separate axis for them but anyone who has driven go-karts knows how much fun using them at the same time can be :) You can even see these are separate properties in the physics engine The code simplifies to:
So yes, I fully agree with you, for normal top down games where you just need a value between -1.0 and 0.0 for character movement, you need two lines of code where one will do. But at the added price of far more setup in the input mapping. There is no right or wrong here imho. Both have pro's and con's. What I like about Grouds suggestion is that it doesn't change anything in the input mapping, it just adds to the information you can gather about the state of controllers. I also feel it makes it easier to setup more complex control situation such as my acceleration/braking scenario where it is more likely you're dealing with two separate axis. |
@AndreaCatania Yeah, but this solution complexifies the mapping system by introducing actions that can have two different directions. In you case, if you want to map a key to a double-direction action, you have to map two keys under the action. As an example, for your move_x action you would have to map the left key as negative and the right key as positive, while both are under the same action. It makes the input mapping confuse just for removing 2 lines of code. While, let's be honest, you usually have those lines only once in your code. |
btw
= win :) |
Did not look much at this as I have no connectivity (will hopefully do
tomorrow)..
If this is just for axis, api should be get_axis_value and we should have a
different category for naming and remaping axes, instead of trying to fit
it to the action system IMO
…On Feb 18, 2018 19:53, "Bastiaan Olij" ***@***.***> wrote:
btw
motion.y = get_action_value("move_down") - get_action_value("move_up") # in 2D, other way around in 3D ;)
motion.x = get_action_value("move_right") - get_action_value("move_left")
= win :)
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#16797 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AF-Z27WOSRGEdkW0u70o8hFI1npf5wvsks5tWKnLgaJpZM4SJxpL>
.
|
We already have something to retreive an axis but it does not solves the initial problem of having to deal with code for actions (=keys pressed or not) and joysticks split into two. I think my solution solve the problem pretty well, but well, I'm not unbiased. ^^ |
@reduz having just added both keyboard support and axis support to my demo racing game (and it being indirectly responsible for @AndreaCatania to spend time on this I think) I would beg to differ. I think enhancing the action system is worth considering as it would simplify a lot of coding. I do like @groud suggested approach most so far :) |
But an action is an action, it's something that happened at a given time.
This is not an action, so it's confusing. I prefer it's named according to
what it really is.
…On Feb 18, 2018 20:07, "Bastiaan Olij" ***@***.***> wrote:
@reduz <https://github.com/reduz> having just added both keyboard support
and axis support to my demo racing game (and it being indirectly
responsible for @AndreaCatania <https://github.com/andreacatania> to
spend time on this I think) I would beg to differ. I think enhancing the
action system is worth considering as it would simplify a lot of coding. I
do like @groud <https://github.com/groud> suggested approach most so far
:)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16797 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AF-Z23_gOBW1nTWh64T2JijNmn_SXFBkks5tWK1NgaJpZM4SJxpL>
.
|
@reduz I think that is the problem. Semantically you are absolutely correct. This change makes it no longer purely an action based thing as in "if the user triggers action A, B should happen". But when you're looking at it from a practical perspective, even today it is already much more then that. Look at this code fragment that is in nearly every Godot platform game:
That already is "not an action". We're already "abusing" the action system here because practically speaking the action system allows us to define keyboard, mouse and joystick mappings in a way that we can write code that doesn't make us worry about the input device the user is using. The case may thus be that the action system is wrongly named not that this small change is out of place. While it is used for action based events ("a jump action triggering a jump event which initiates the jump action") it is used for so much more. |
But this was never a purely action based system, it's part of the Input
system.
…On Feb 18, 2018 20:21, "Bastiaan Olij" ***@***.***> wrote:
But an action is an action, it's something that happened at a given time.
This is not an action, so it's confusing. I prefer it's named according to
what it really is.
@reduz <https://github.com/reduz> I think that is the problem.
Semantically you are absolutely correct. This change makes it no longer
purely an action based thing as in "if the user triggers action A, B should
happen". But when you're looking at it from a practical perspective, even
today it is already much more then that. Look at this code fragment that is
in nearly every Godot platform game:
func _process(delta):
if Input.get_action_pressed("move_left"):
position.x = position.x - (delta * MOVE_SPEED)
elif Input.get_action_pressed("move_right"):
position.x = position.x + (delta * MOVE_SPEED)
That already is "not an action". We're already "abusing" the action system
here because practically speaking the action system allows us to define
keyboard, mouse and joystick mappings in a way that we can write code that
doesn't make us worry about the input device the user is using.
The case may thus be that the action system is wrongly named not that this
small change is not out of place. While it is used for action based events
("a jump action triggering a jump event which initiates the jump action")
it is used for so much more.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16797 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AF-Z20tWP2lKrL1rm9QLUy3NFV3TXrd9ks5tWLCOgaJpZM4SJxpL>
.
|
@reduz exactly, so what is wrong with adding the ability to not just get the on/off is_pressed value of mapped input but also get a value between 0.0 - 1.0 to turn the input map into something you can use for analogue input? It seems convoluted to me to build a new mapping system for analogue input when the system we have provides us with 95% of what we need. Or am I barking up the wrong tree and we're actually talking about the same thing, you just want the name of the function to be different? |
@reduz note btw, that this is not just for axis, the whole goal here is to map an action like "move forward" to a key (W), an axis (axis 1 positive) and maybe even mouse movement, exactly like the current mapping already does, but instead of getting a true/false, we get a value between 0.0 and 1.0. When the key is pressed it will return 1.0, if the joystick is used, the value is between 0.0 and 1.0 |
Too much messages to reply 😱 First thing is that I never said that I've removed the old actions APIs (that would be a stupid thing to do IMO since the actions APIs are fine) So this is the list of supported API in this moment with this PR
As you can see I've only added two APIs this mean that all old approach still valid and correct to use, but in addition you will have (if required) the possibility to get value of the axis. For example now is possible to have a Joypad where the buttons has a pressure, now we can map it easily. Reply:
@BastiaanOlij As I've said above you can and you MUST continue to use this approach:
This PR has the only objective to add a way to map Sticks inside the current actions in order to have the same mapping for all kind of devices.
@groud You are saying that this solution makes things confusing and less consistent, but I never saw a single post that complain this approach to UE4, and also people seems happy for this implementation on YouTube and also Twitter. I think that this approach makes things a lot easier. Doesn't exists a single game, or at least very few, where you get a benefit to separate the two axis of joystick in other four. The sticks are always used to move the camera position or orientation and the range -1 to 1 is perfect for this scenario. Also I want to talk about range of inputs, with this PR you will get a different range according to the input, for example as discussed before if you have a Stick the axis range will be -1 to 1 but if you have a shoulder trigger (for example L2) the range will be 0 to 1. So the function get_action_axis_value will return always the natural value that you expect, or 0 if the button doesn't simulate the axis.
@reduz I thing that move a stick is also an action, and you can use is_action_just_changed to know if this action is just triggered or not. However I would be glad if you compile the code and you test it with your current project and then would bring to me the problems that you have found, so I can try to figure out how I can improve it |
Just a few more cents from my side after talking somewhat more in depth with Odino to kick ideas around and understand his reasoning better. I think Odino's approach does make more sense to me now but I see the sense in a number of the things raised especially in it potentially being confusing in the action system. I think Reduz probably highlighted this best when we discussed if this should be part of the action system. While there is overlap in this all being about input, in the long run this is about what makes sense for building a game that uses analogue input with a fallback to a keyboard. So here is my suggestion for making this better:
Just my take on it at this point in time :) |
This comment has been minimized.
This comment has been minimized.
Well yeah, having to write something like
But yeah, finally the question is whether if we want users to write a little bit more code, or if we want them to spend more time in the Axes/Actions menu. I personnaly hate having to remap inputs when my gameplay changes, that's why I prefer my solution. |
I guess I am overestimating the benefits of a -1 to 1 range, something like "up minus down" does seem simple enough to program. Anyway, anything is better than booleans for my use case. I would still like to state that, for keyboards, a |
Sure, that kind of things could be modified in the InputMap menu. Anyway, to be honest this seems to be something quite controversial. I believe we should organize a vote or something like that, to see what people think about both idea. |
Currently this implementation does it |
Just wanted to say that this is exactly what I need and would save me a lot of work with my game Vegg. Both implementation concepts of @AndreaCatania and @groud seem to be good and even though one would fit better for my current game the other one would require just minimal workarounds. And for other games it would be the other way around. Might even port my game from Godot 2 to Godot 3 if this gets merged. |
8383a4b
to
3a42185
Compare
In the last commit I've improved input mapping for multiplayer games by adding per controller mapping. Check this video and let me know what do you think about: https://youtu.be/vCBu6CSsnjo |
Why was it closed? (Superseded by another PR I guess? But it would be good to mention here for clarity) |
Akien ye correct, I closed it because reduz like more the approach of splitting joy axis, so this #16902 PR will be merged |
For what's worth it, i prefer having axis mapped from -1 to 1. I don't see the use case for [0,1]. I actually think it would be annoying to work with. |
[0,1] makes sense for throttle or trigger inputs, but of course [0,1] fits within [-1,1] |
@raymoo In my solution the shoulders trigger are mapped 0 - 1 and usually are used for throttle in car games. So as i said before no real need to separate the axis. |
What about the multiplayer controller mapping? I like the per controller mapping, it would be very useful for making local multiplayer games. |
I like it too, but reduz said that is too simple work around it that is useless implement it. |
Actually, you can map and define per-player actions by doing something like that:
|
I know you can do that, but it still would be nice to have it done semi-automatically like it does in this PR. I've done that sort of mapping before whenever I've tried to implement local multiplayer. It seemed like a pretty nice addition, without having to manually type in all of the names, but it's not that hard to work around it. |
Now that the other PR for inputs has been merged, it would probably be best to open a new PR that only includes changes we'd like to make on top of it. |
Regarding multiplayer controller mapping per player thing...
I've proposed a somewhat refactored approach regarding how input states can be decoupled from input polling system here: godotengine/godot-proposals#104. The proposal is centered around having an easier way to control multiple characters within a game session, not necessarily multiple players per session, but there may be some connections which I can't see for myself being useful what was proposed here. Talking specifically, my proposal aims to provide some encapsulated input state which can basically reflect the global input state the same way you'd use an My current implementation #35240 only has a master instance of an # Local single-player
var state = Input.state as InputState
var player = state.get_id()
# ... input handling code for the player goes here ...
if state.is_action_pressed("shoot"):
pass vs # Local multi-player
for i in Input.get_local_state_count():
var state = Input.get_local_state(i) as InputState
var player = state.get_id()
# ... input handling code for EACH player/controller goes here ...
match player:
0:
if state.is_action_pressed("shoot"):
shoot_apples()
1:
if state.is_action_pressed("shoot"):
spam_eggs()
# etc. Thoughts? |
With this commit I've added a way to map stick (axis) input inside Godot actions.
Basically now is possible to map together axis and buttons in order to simplify the life of developer and allow to change bindings at runtime easily.
Please check this video to see it in action: https://youtu.be/tKd7rEfwpIo